Refactor Option Builder #414
Conversation
add test code for the modified changes
Refactored the FunctionCallingOptions interface by separating it into two distinct interfaces: FunctionCallingOptions. java and PortableFunctionCallingOptions.java. This change aims to enhance modularity and clarify the responsibilities of each interface in the system.
Hidden Implementation and Enforced Access through an Interface.
Hidden Implementation and Enforced Access through an Interface.
Hidden Implementation and Enforced Access through an Interface.
Modified code that was using the existing builder.
Hidden Implementation and Enforced Access through an Interface.
* ChatBuilderTests.java * ImageOptionsTests.java * FunctionCallingOptionsTests.java
Adjusted sections where modifications to injected objects could potentially cause issues.
Modified to facilitate documentation through the use of the @nonnull annotation.
- Add missing license header
- Establish a new "spring-ai-retry" project, implementing a default HTTP error handler, RetryTemplate, and handling both Transient and Non-Transient Exceptions. - Streamline existing clients (e.g., OpenAI and MistralAI) to utilize "spring-ai-retry." - Integrate retry auto-configuration with customizable properties, extending it to OpenAI and MistralAI Auto-Configs. - Allow configuration of RetryTemplate and ResponseErrorHandler for various clients, including OpenAIChatClient, OpenAiEmbeddingClient, OpenAiAudioTranscriptionCline, OpenAiImageClient, MistralAiChatClient, and MistralAiEmbeddingClient. - Add tests for default RestTemplate and ResponseErrorHandler configurations in OpenAI and MistralAI. - Introduce new retry auto-config properties: "onClientErrors" and "onHttpCodes". - Implement tests for retry auto-config properties. - Generate missing license headers.
This allows to bootstrap multiple instances for AI clients or vectors stores in the same application context.
Also update the embeding and chat api diagrams
This reverts commit 231e9cc. Revert Branch
memory Optimized
Modified to facilitate documentation through the use of the @nonnull annotation.
The code was already well-crafted, so no immediate modifications were evident. However, to maintain consistency across the entire builder API, it was necessary to make adjustments to the from method.
Merge refactor_builder_vectorstore into refactor_openai_builder
Seperated implementation and interface
Seperated Option Object, Property Object
Apply Changes
Seperated implementation and interface
Seperated Option Object, Property Object
Apply Changes
Seperated implementation and interface
Seperated Option Object, Property Object
Apply Changes
Seperated implementation and interface
Seperated Option Object, Property Object
Apply Changes
Merge Main into refactor_openai_builder Resolved Conflict ChatOptionsTests.java
Merge main branch Resolved Conflict
|
Thank you, this is an area of the api I would like to review. |
|
There are some good things—no one is arguing against immutability—but the PR does too much, and there are some choices that I don't understand or disagree with. Let's see how we can split this up into more fine-grained issues. I'll take your bullet points in the PR comment one by one.
The convention that I've found in Spring is not to have a top-level class with the suffix As an example, this PR has a top-level: public class OpenAiAudioTranscriptionOptionsBuilder {But the current code has: public static Builder builder() {
return new Builder();
}
public static class Builder {
protected OpenAiAudioTranscriptionOptions options;
}I do not want to break this stylistic convention, as we should adhere to the Spring ecosystem conventions. I do see that sometimes Spring projects create a builder interface and then have a DefaultBuilder implementation. For example, UriHandlerFilter in Spring Framework does this. I think that's a good idea, Also, we haven’t been consistent: for example, ChatOptionsBuilder is a top-level class outside the ChatOptions compilation unit. Adding tests for the options builder class is a good idea. Created issue #1592 to discuss.
I see that our current options builder already takes an existing options object, so this is already handled.
There is already a class for this, it doesn’t follow the pattern mentioned above (i.e., the builder class is outside the class it builds).
We are currently looking to remove the Boot dependency from spring-ai-core, which mostly boils down to removing @NestedConfigurationProperty. We're not sure about the cleanest approach yet, so we are experimenting to avoid adding too much boilerplate. This PR doesn't address that concern and also I don't see the advantage over the implementation you provided. The separation will happen, and any Boot-specific implementation to achieve this will occur in the auto-configuration classes where the @ConfigurationProperties are located. Created #1591 for this topic. I appreciate the contributions, but i will close this issue and the points raised can be tackled through the new issues. |
This change aims to conceal the implementation and facilitate handling option objects through the interface.
And enforcing immutability to ensure greater safety in multi-threaded environments.
Add Builder and Nested Private Implemented Class.
Add builder() to Create New Object from existing.
Add PortableFunctionCallingOptions to extract ChatOptions, FunctionsOptions
Open AI
To separate the responsibilities of objects, implement a dedicated object for Properties binding.
Hide the implementation and only allow access through the interface to increase extensibility.
Thanks